-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add check for building docsite #14406
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it!
jobs: | ||
docsite-build: | ||
name: docsite test build | ||
runs-on: ubuntu-latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using Fedora? One of my goals is to start using Fedora in all our tests. It'd be cool to start here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners
The choices are Ubuntu, Windows, or Mac.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nuts. I could swear I noticed Fedora in another repo somewhere and it's kind of been on my to-do list to start looking at the switch. sorry for the noise.
.github/workflows/docs.yml
Outdated
- name: Assure docs can be built | ||
run: tox -e docs | ||
|
||
- name: Upload html build as artifact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this step? We can enable PR previews on Read The Docs when we set up hosting. Admittedly I've put this same step in other places but I'm thinking about disabling it.
If we do want to keep it, how about reducing the retention period so we don't have loads of doc builds hanging around and taking up space. The default is 90 days but we could drop that to ~7. https://github.com/actions/upload-artifact#retention-period
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I really wanted your feedback on this. Yes, I'm totally fine to get rid of this and even if we did keep it I agree with dropping the retention time.
I want to defer to whatever method will be used to publish them. So if the readthedocs integration is set up on their side, then it makes no sense to have this. I had in my head that maybe we would have this step, and elsewhere another step that uploaded it to the readthedocs API. That sounds unlikely so I'll push a commit to remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlanCoding Awesome. I can send a PR with the RTD config today and set up the server-side integration with you and TVo as maintainers. Then we can add it to the ansible namespace. Sound OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand all the proper nouns you reference there, but in general it sounds good. I assume that publishing these is next in your agenda and we can sync up in some other communication channels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies, I have a habit of using "RTD" instead of "Read The Docs". I can ping you in Matrix to sync up on the publishing tomorrow if that works.
SUMMARY
To make sure people don't break the build of the docsite
ping @oraNod
ISSUE TYPE
COMPONENT NAME